Skip to content

Fix #1124: close the pre-steal-inode race, inode-reuse/non-preserving-rename gap, and hand-back clobber in _lock_try_steal - #1126

Open
fdaviddpt wants to merge 3 commits into
mainfrom
fix/1124
Open

fdaviddpt wants to merge 3 commits into
mainfrom
fix/1124

Conversation

@fdaviddpt

Copy link
Copy Markdown
Contributor

Summary

Three self-contained fixes to _lock_try_steal / _lock_file_inode / hand-back in
scripts/lib-lock.sh, closing the three paths #1124 found after #1123:

  1. Pre-steal inode snapshot timing -- the "before" inode was captured after the first
    pid read and the age check, leaving a window where a racing contender's full
    steal-and-rewrite cycle could land and be mistaken for the unchanged original
    placeholder, producing two simultaneous holders. Fixed by moving the snapshot to the
    top of the function, before the first read.
  2. Rename does not preserve inode identity everywhere -- _lock_file_inode assumed
    rename preserves identity, false on FAT/exFAT under Cygwin/MSYS, SMB noserverino, FUSE
    without use_ino (Lock check hangs on a dataless (cloud-only) pid file when .remember/ lives in a synced folder; every save then reports "another save holds the lock" #1103's own setup). Fixed with a new self-test,
    _lock_rename_preserves_identity, that detects this and falls back to the existing
    content re-read.
  3. Hand-back clobber -- every hand-back mv could clobber a pid another process
    legitimately wrote. Fixed with a new _lock_handback helper using noclobber for the
    write.

Two self-review rounds followed (Explore + oss:auditor, spawned independently against
the diff each time, tree_snapshot clean before and after every spawn) and fixed four
further defects the first commit's own fix introduced or left open:

  • The identity-probe helper, run inside the lock directory, reset that directory's own
    staleness-tracking mtime on every steal attempt, reopening the save-session.sh: a hung save holds save.lock forever, and every later save is skipped #1101/Lock check hangs on a dataless (cloud-only) pid file when .remember/ lives in a synced folder; every save then reports "another save holds the lock" #1103 outage via
    the helper meant to avoid it. Fixed by moving the probe to the lock directory's parent.
  • The hand-back fallback was still a bare unguarded mv, and every call site had
    already attempted its own read. Fixed by threading the already-read content through as
    an optional argument and falling to a guarded (destination-confirmed-absent) restore.
  • That same fallback then conflated "no read attempted" with "read succeeded and the pid
    file is legitimately empty," routing a real empty value into the weaker fallback.
    Fixed by testing argument count instead of value emptiness.
  • The identity-probe's parent-directory derivation stripped only one trailing slash, so
    a caller-supplied path with a trailing slash got parent == dir, silently reopening
    the mtime-reset finding above for that input shape. Fixed with a portable (bash
    3.2-safe) loop that strips all trailing slashes.

Two low-severity residuals are documented in code comments and logged under trap.d/
rather than fixed, by agreement of both independent reviewers in both rounds, and both
are below-bar:

  • trap.d/1124.steal-probe-litter-on-kill.md -- new .steal-probe.* files have no
    reaping logic if a process is killed mid-probe.
  • trap.d/1124.handback-fallback-residual-toctou.md -- the guarded-mv fallback itself is
    check-then-act, not atomic; narrower TOCTOU window than the original bare mv, not
    zero. Neither blocks this pull request per both reviewers' judgment in both rounds.

The review loop was stopped after this second self-review round by deliberate judgment
rather than running a third: fix_commit_scope.py still said needs-second-pass (4 files
touched), but both independent reviewers in the second round converged on the same core
finding independently, and the one remaining documented gap (the fallback's own TOCTOU)
was already ranked non-blocking by both reviewers and is the kind of residual this file
already accepts elsewhere (pid-recycling, inode-portability).

Cross-platform claims

  • The mtime-reset fix is observed directly on darwin/APFS via the regression test;
    the underlying reasoning (directory-mtime-on-entry-change is POSIX-universal) is
    portable, but this was never run on Linux, Windows, Cygwin/MSYS, or an actual
    FAT/exFAT/SMB/FUSE-without-use_ino mount.
  • The premise that rename does not preserve identity on those filesystems is
    reasoned from documentation, never observed on a real instance of any of them --
    this repo's CI matrix (ubuntu-latest/macos-latest/windows-latest, i.e. ext4/APFS/NTFS)
    does not cover any of them either.
  • bash 3.2 compatibility of the new trailing-slash-stripping loop is reasoned from
    bash version history, not observed against an actual bash 3.2 interpreter.

Also noted, unrelated to this fix

Closes #1124

🤖 Generated with Claude Code

[AI-generated]

…ving-rename gap, and the hand-back clobber in _lock_try_steal

A code review of merged #1123 found three more gaps in the same function's
stale-lock takeover: the "before" inode snapshot was captured after both the
first pid read and the age check, leaving a window for another contender's
full steal cycle to complete and be mistaken for the unchanged original
placeholder; _lock_file_inode assumes rename preserves identity, which is
false on FAT/exFAT under Cygwin/MSYS, SMB with noserverino, and FUSE without
use_ino, where the identity check would hand back forever; and every
hand-back mv could silently overwrite a pid another process had already
written.

The snapshot now runs before the first read; a one-time per-steal self-test
(_lock_rename_preserves_identity) skips the identity comparison on a
filesystem where it cannot answer meaningfully, falling back to the content
re-read alone rather than refusing forever; and hand-back now reads the
claim's own content back and writes it with noclobber, the same atomicity
the real steal writes already use.

Co-Authored-By: Max <noreply>
…ng lock staleness, and stop the hand-back's own fallback from clobbering

Two spawned reviewers (Explore and oss:auditor) against the first #1124
commit found three more gaps, all in code this same commit introduced:

- The new filesystem probe (_lock_rename_preserves_identity) ran inside
  the lock directory itself. Creating/renaming/removing any directory
  entry updates that directory's mtime, which is exactly what
  _lock_dir_age reads to judge staleness everywhere in this file -- so
  the probe was resetting the clock on every single steal attempt,
  reproduced directly (before=120 after=0), which would have let a
  permanently stuck lock look freshly touched forever (reopening
  #1101/#1103). The probe now runs in the lock directory's parent
  instead, which updates the parent's mtime, never the lock dir's.

- _lock_handback's own "could not read the claim's content back"
  fallback was still a bare, unguarded mv -- the exact clobbering
  pattern the noclobber write above it exists to close, just left open
  on the unreadable-content path, which is the realistic one most
  hand-backs take. It now falls back to a guarded restore (only mv when
  the destination is confirmed still absent) in every case, readable
  content or not.

- Every call site had already attempted its own read of the claim
  before ever calling _lock_handback, so the helper re-reading it a
  second time doubled the read-timeout latency on exactly the unreadable
  path. It now takes content the caller already read as an optional
  argument instead of re-reading.

Added a regression test reproducing the staleness-clock bug directly
(red on the pre-fix probe location, green after), and a test driving
_lock_handback's noclobber write with a destination that already holds
a different, legitimate pid. A fourth, lower-severity finding (orphaned
probe litter if a process is killed mid-probe) is documented in the code
and logged to trap.d/ rather than fixed under this issue's time budget.

Co-Authored-By: Max <noreply>
…nd the trailing-slash probe-location bug

Second review round over the previous follow-up commit (same two spawns,
over the follow-up diff alone, per fix_commit_scope.py's needs-second-pass
verdict) found two more real gaps in that commit's own new code,
independently from both reviewers:

- _lock_handback used "[ -z "$_content" ]" to decide whether content was
  known. A pid file legitimately read as empty (a holder mid-acquisition,
  or one that died before writing its pid -- both real, documented cases)
  produces that same empty string from a SUCCESSFUL read, so it was
  routed into the weaker fallback restore path instead of the atomic
  noclobber write every other known value gets. Fixed by testing "$#"
  (was a 3rd argument supplied at all) instead of the value's emptiness.

- _lock_rename_preserves_identity's parent derivation ("${_dir%/*}")
  strips only one trailing slash from a path that already ends in "/",
  landing back on the SAME directory rather than its parent -- silently
  reopening the staleness-clock bug the relocation exists to close, for
  any caller that happens to pass a trailing slash. Fixed with a
  portable (bash 3.2-safe) case/glob loop that strips all trailing
  slashes before deriving the parent.

Both auditor findings about the fallback's own residual check-then-act
window (narrower than the bare mv it replaced, not zero) are documented
in the code and logged to trap.d/ rather than fixed, since closing it
needs an atomic "create only if absent" primitive this function does not
have for unknown content.

Two new regression tests, both red on the pre-fix code and green after:
one reproducing the empty-content misroute by making the claim's own
on-disk bytes differ from the content passed, one reproducing the
trailing-slash bug directly against _lock_dir_age.

Co-Authored-By: Max <noreply>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Lock takeover: three remaining two-holder/outage paths in _lock_try_steal after #1123

1 participant